fix(keyring-controller): Only drop target keyring on removeAccount#7670
Merged
matthiasgeihs merged 10 commits intomainfrom Jan 26, 2026
Merged
fix(keyring-controller): Only drop target keyring on removeAccount#7670matthiasgeihs merged 10 commits intomainfrom
removeAccount#7670matthiasgeihs merged 10 commits intomainfrom
Conversation
matthiasgeihs
force-pushed
the
mg/keyring/limit-clear-keyrings
branch
from
eaf1119 to
b96e9e1
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
mikesposito
previously approved these changes
Jan 20, 2026
Member
mikesposito
left a comment
mikesposito
left a comment
There was a problem hiding this comment.
Some small suggestions, but overall looks good.
Comment on lines
+1651
to
+1658
| // Import an account, creating a Simple keyring with 1 account | ||
| const importedAccount = await controller.importAccountWithStrategy( | ||
| AccountImportStrategy.privateKey, | ||
| [privateKey], | ||
| ); | ||
|
|
||
| // Add an empty Simple keyring (no accounts) | ||
| await controller.addNewKeyring(KeyringTypes.simple); |
Member
There was a problem hiding this comment.
Passing a custom vault to withController would avoid the need of using importAccountWithStrategy and addNewKeyring, making the test scenario clearer as we can directly see what's in the vault when the test is run. Though this works as well, so you can consider this a NIT suggestion
Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com>
Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation
Before,
removeAccountwould clear all empty keyrings from the keyring controller, leading to undesirable side effects. (E.g., when a keyring has been initialized as empty, it would be dropped here)Changing
removeAccountto only drop the target keyring if its last account has been removed. (And no longer touch the other keyrings.)Also fixes an issue where Snap Keyring accounts would not be removed in order (because their
removeAccountfunction is async, but this was previously not awaited).References
Checklist
Note
Refines account removal behavior and hardens keyring lookups.
removeAccountto awaitkeyring.removeAccountand remove only the targeted keyring when it becomes empty; delete#removeEmptyKeyrings#findKeyringIndexForAccountwith case-insensitive address matching; prevents unintended primary keyring deletion with non-normalized addressesgetKeyringForAccountto use the new lookup helperremoveAccountcompletes before keyring removalWritten by Cursor Bugbot for commit 074a6c1. This will update automatically on new commits. Configure here.